Skip to content

fix(workflows): fail a fan-in step whose output is not a mapping - #3887

Merged
mnriem merged 1 commit into
github:mainfrom
jawwad-ali:fix/fanin-nonmapping-output
Jul 31, 2026
Merged

fix(workflows): fail a fan-in step whose output is not a mapping#3887
mnriem merged 1 commit into
github:mainfrom
jawwad-ali:fix/fanin-nonmapping-output

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

FanInStep.execute began with:

output_config = config.get("output") or {}
if not isinstance(output_config, dict):
    output_config = {}

Any non-mapping output was silently replaced with {} and the step still returned COMPLETED. Every aggregation key the author declared vanished, and downstream {{ steps.<id>.output.<key> }} resolved to None and interpolated as an empty string.

Note also that x or {} masked the falsy shapes ([], false, 0, '') before the isinstance check even ran.

Reproduction on current main (81bf741)

output=[]       -> status=completed  error=None
output=False    -> status=completed  error=None
output=0        -> status=completed  error=None
output=''       -> status=completed  error=None
output=['a']    -> status=completed  error=None
output='oops'   -> status=completed  error=None
output=5        -> status=completed  error=None

This is already a known flaw — in this file

validate rejects a non-mapping output, and its comment names the execute-side bug outright:

output = config.get("output")
if output is not None and not isinstance(output, dict):
    # execute() silently coerces a non-mapping output to {}, so the
    # author's declared aggregation keys would vanish with no error.
    # Reject at validation, mirroring the command-step (#3262) fix.

The engine does not auto-validate before execute (see WorkflowEngine.load_workflow), so on an unvalidated run that comment describes exactly what happens. This PR closes the execute-side half.

Fix

Fail the step with validate()'s own message, mirroring the wait_for guard a few lines below in the same method:

output_config = config.get("output")
if output_config is None:
    output_config = {}
elif not isinstance(output_config, dict):
    return StepResult(status=StepStatus.FAILED, error=..., output={"results": []})

This is the same "silent empty result + COMPLETED" class the wait_for guards in this method already reject, and the same shape as the fan-out non-list items guard.

No breaking change. An explicit output: (YAML null) stays valid, matching validate — pinned by a second new test. A mapping behaves exactly as before. The only inputs whose behaviour changes are ones that silently produced an empty aggregation.

Verification

  • Fail-before / pass-after: the seven parametrized cases fail on unpatched src and pass with the fix.
  • tests/test_workflows.py scoped regression: failure set identical to the clean-main baseline captured on 81bf741 (20 pre-existing in this file, all Windows symlink-privilege).
  • uvx ruff@0.15.0 check src tests → clean

Tests go into the existing TestFanInStep, beside test_execute_non_list_wait_for_fails_loudly — the sibling guard whose docstring describes the same wiring bug.


Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current main.

execute() did:

    output_config = config.get("output") or {}
    if not isinstance(output_config, dict):
        output_config = {}

so every non-mapping `output` was silently discarded and the step still
returned COMPLETED — every declared aggregation key vanished, and
downstream `{{ steps.<id>.output.<key> }}` resolved to None and
interpolated as an empty string:

  output=[]       -> completed, error=None
  output=False    -> completed, error=None
  output=0        -> completed, error=None
  output=''       -> completed, error=None
  output=['a']    -> completed, error=None
  output='oops'   -> completed, error=None
  output=5        -> completed, error=None

`validate` already rejects this and its comment names the flaw exactly:
"execute() silently coerces a non-mapping output to {}, so the author's
declared aggregation keys would vanish with no error." The engine does not
auto-validate before execute(), so on an unvalidated run that is what
happened — and `x or {}` masked the falsy shapes before the isinstance
check even ran.

Fail loudly with validate()'s own message, mirroring the `wait_for` guard
in the same method. An explicit `output:` (YAML null) stays valid.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Ready to approve

The focused fix matches validation behavior and includes adequate regression coverage.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Ensures fan-in steps fail clearly when output is not a mapping while preserving null-output behavior.

Changes:

  • Adds runtime validation for malformed fan-in output configuration.
  • Adds regression coverage for invalid and null outputs.
File summaries
File Description
src/specify_cli/workflows/steps/fan_in/__init__.py Returns a failed result for non-mapping output values.
tests/test_workflows.py Tests invalid output types and explicit null handling.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

@mnriem
mnriem merged commit 521020b into github:main Jul 31, 2026
14 checks passed
@mnriem

mnriem commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants